Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add no-pic debug flag to disable position independent code #11995

Closed
wants to merge 1 commit into from

Conversation

bharrisau
Copy link
Contributor

Probably don't want to be adding to the number of debug flags, but there seemed to be interest in adding this in #10942.

From the LLVM source DynamicNoPIC is "Relocatable external references, non-relocatable code". On platforms it isn't supported on it becomes either RelocStatic or RelocPIC.

Again, I haven't written a test. I couldn't see any existing ones for PIC stuff. Passes existing tests though.

@thestinger
Copy link
Contributor

Can you add this as a regular flag, rather than a debug one? The documentation needs to be updated too. Does it make sense to also expose RelocStatic?

@bharrisau
Copy link
Contributor Author

I'm not sure what the use cases are. I just ran out of ways to build Cortex-M code in a sane way (gets hard quickly with rlibs) so I've started implementing all the small things that looked like they had consensus but were closed for other reasons.

I'll look at moving it into a normal flag and documenting it.

@alexcrichton
Copy link
Member

I'm a little uneasy about this in either -Z or at the top-level, but I've put my concerns in #12000. I also agree with @thestinger that if we're exposing more than one codegen option we should probably expose them all

@thestinger
Copy link
Contributor

@alexcrichton: clang and gcc expose this at the top-level as it's a useful thing to ask for. On some platforms, it may make sense to disable position independent code as an optimization. It reduces the cost of accessing a global.

@alexcrichton
Copy link
Member

It's true, but man gcc and man clang is also very sprawling. Additionally clang -help prints a very large number of things. We seem to have been generally following the clang/gcc patterns, but we have pushback for consolidating them (#7791).

One thing I also forgot to mention is how crates link together. If I disable relocatable code, can I generate a dynamic library? Also, if one rlib has no relocatable code, and mine does, can I link to it? In general I think we want to give errors before running the linker. I'm unsure of how all these modes play together, so I'm fine with merging and seeing how this all plays out.

@thestinger
Copy link
Contributor

I think what you're allowed to do will depend heavily on the platform and the native libraries you're linking against. I don't think it's worth trying to add sensible errors at a Rust frontend level because it's only used in rare edge cases.

@alexcrichton
Copy link
Member

I'd be down with that.

@bharrisau, taking a leaf out of LLVM's book, I think that this should probably add a --code-model=<foo> flag to rustc and then just pick the right one during codegen (with sess.err("foo") if it's invalid).

@thestinger
Copy link
Contributor

+1, lets copy llc instead of clang as it wasn't designed for backwards compatibility

@alexcrichton
Copy link
Member

We now have -C, so the codegen flags would be a good place to put this under: -C code-model=foo

@alexcrichton
Copy link
Member

Closing due to inactivity, but I'd love to see this implemented!

bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 25, 2022
…eykril

Generate enum variant assist

So, this is kind of a weird PR!

I'm a complete newcomer to the `rust-analyzer` codebase, and so I browsed the "good first issue" tag, and found rust-lang#11635. Then I found two separate folks had taken stabs at it, most recently `@maartenflippo` — and there had been a review 3 days ago, but no activity in a little while, and the PR needed to be rebased since the crates were renamed from `snake_case` to `kebab-case`.

So to get acquainted with the codebase I typed this PR by hand, looking at the diff in rust-lang#11995, and I also added a doc-test (that passes).

I haven't taken into account the comments `@Veykril` left in rust-lang#11995, but I don't want to steal any of `@maartenflippo's` thunder! Closing this PR is perfectly fine. Or Maarten could use it as a "restart point"? Or I could finish it up, whichever feels best to everyone.

I think what remains to be done in this PR, at least, is:

  * [x] Only disable the "generate function" assist if the name is `PascalCase`
  * [x] Only enable the "generate variant" assistant if the name is `PascalCase`
  * [x] Simplify with `adt.source()` as mentioned here: rust-lang/rust-analyzer#11995 (comment)
  * [ ] Add more tests for edge cases? Are there cases where simply adding one more indent level than the enum's indent level is not good enough? Some nested trickery I'm not thinking of right now?

Anyway. This PR can go in any direction. You can tell me "no, tackle your own issue!" And I'll go do that and still be happy I got to take a look at rust-analyzer some by doing this. Or you can tell me "okay, now _you_ finish it", and I guess I'll try and finish it :)

Closes rust-lang#11635
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants